-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minimal Bubbling Observers #13991
Minimal Bubbling Observers #13991
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few typos on some internal test items (awesome tests btw!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too familiar with the observer code (or ECS details in general), but I didn't spot any obvious issues based on a quick look. I mainly went through the docs and how this works at a high level.
The core implementation looks solid, and will definitely be highly useful for picking and UI, as well as many general patterns. I like that you can implement custom traversal: for example in bevy_xpbd
, I have a ColliderParent
component, and I could use it to propagate collision events from child colliders directly to rigid body entities.
I do agree with @MrGVSV that the current terminology is potentially misleading. Bubbling in my mind always goes up in the hierarchy, which is not necessarily the case for us if we allow custom traversal.
Overall I'd like to see some improvements to the documentation here: What is event bubbling? What could it be useful for? How do I stop propagation? What is Event::Traverse
actually used for? How can I implement my own traversal? These are largely answered in the PR description, but I'd like it in the actual documentation.
I would like advice on how best to document this, since it doesn't really expose anything publicly by default, except for perhaps the new The docs entry-point for observers seems to be the |
My preference is to primarily cover this on |
I resolved all the issues identified in the reviews and I was in the process of porting over the examples from I'll be rewriting this a bit over the next week to fix this issue. |
Completely non-blocking - have you done any sort of performance profiling? I'm curious how this scales. |
I have not yet profiled. I am expecting it to be somewhat slower. Will port over the eventlistener stress test once I fix the footguns. |
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very elegant :)
@@ -662,4 +725,210 @@ mod tests { | |||
world.flush(); | |||
assert_eq!(1, world.resource::<R>().0); | |||
} | |||
|
|||
#[test] | |||
fn observer_propagating() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple fix is a Vec<str>
where we can push observer "labels" when they execute and then verify with vec!["obs1", "obs2"]
it's only tests so it doesn't need to be "proper"
would that be alright?
Great, thank you! I prefer the bool as well, that's how I had it implemented before the first round of feedback. I'll get this cleaned up, benchmarked, and ready for merge by next monday then. When this is done, I'll spin up some issues for follow up work that we uncovered:
|
Addressed review comments, added a benchmark, cleaned things up a bit. Preliminary profiling indicates this is a regression compared to the best case of If this passes tests I think it's good for merge. |
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1657 if you'd like to help out. |
Objective
Add basic bubbling to observers, modeled off
bevy_eventlistener
.Solution
Traversal
trait for components which point to other entities.TraverseNone: Traversal
component which cannot be constructed.Traversal
forParent
.Event
trait now has an associatedTraversal
which defaults toTraverseNone
.bubbling: &mut bool
toTrigger
which can be used to instruct the runner to bubble the event to the entity specified by the event's traversal type.SHOULD_BUBBLE
toEvent
which configures the default bubbling state.Introducing the new associated information directly on
Event
(instead of a newBubblingEvent
trait) lets us dispatch both bubbling and non-bubbling events through the same api.Testing
I have added several unit tests to cover the common bugs I identified during development. Running the unit tests should be enough to validate correctness. The changes effect unsafe portions of the code, but should not change any of the safety assertions.
Changelog
Observers can now bubble up the entity hierarchy! To create a bubbling event, change your
Derive(Event)
to something like the following:You can dispatch a bubbling event using the normal
world.trigger_targets(MyEvent, entity)
.Halting an event mid-bubble can be done using
trigger.propagate(false)
. Events withAUTO_PROPAGATE = false
will not propagate by default, but you can enable it usingtrigger.propagate(true)
.If there are multiple observers attached to a target, they will all be triggered by bubbling. They all share a bubbling state, which can be accessed mutably using
trigger.propagation_mut()
(trigger.propagate
is just sugar for this).You can choose to implement
Traversal
for your own types, if you want to bubble along a different structure than provided bybevy_hierarchy
. Implementers must be careful never to produce loops, because this will cause bevy to hang.Migration Guide
Event
should add associated typeTraverse = TraverseNone
and associated constantAUTO_PROPAGATE = false
;Trigger::new
has new fieldpropagation: &mut Propagation
which provides the bubbling state.ObserverRunner
now takes the same&mut Propagation
as a final parameter.